Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

test_runner: handled change for exposing 'spec' reporter #48202

Closed
wants to merge 3 commits into from
Closed

test_runner: handled change for exposing 'spec' reporter #48202

wants to merge 3 commits into from
+4 −6

Conversation

Sumi0
Copy link

@Sumi0 Sumi0 commented May 27, 2023 •
edited
Loading

Fixes: Issue#48112

Other reporters (dot, tap) by signature are a function while 'spec'
reporter is a ES6 class.

This behaviour of api spec is causing difference in semantics while
consumption since it has not been addressed anywhere in the document
(it has to be instantiated).

Instead of making changes in the signature of spec.js, i have proposed
changes where the 'spec' reporter gets exposed in reporter.js

Refs: reporter/spec.js
Refs: (@line-no:143) test_runner/utils.js

@Sumi0
test_runner: handled change for exposing 'spec' reporter
4ebfc7b 
Other reporters (dot, tap) by signature are a function while 'spec' reporter is a ES6 class.

This behaviour of api spec is causing difference in semantics while consumption since it has not been addressed anywhere in the document (it has to be instantiated).

Instead of making changes in the signature of spec.js, i have proposed changes where the 'spec' reporter gets exposed in reporter.js

Fixes: nodejs#48112
Refs: https://github.com/nodejs/node/blob/main/lib/internal/test_runner/reporter/spec.js
Refs: (@line-no:143) https://github.com/nodejs/node/blob/main/lib/internal/test_runner/utils.js
@nodejs-github-bot
Copy link
Collaborator

Review requested:

  • @nodejs/test_runner

@nodejs-github-bot nodejs-github-bot added needs-ci PRs that need a full CI run. test Issues and PRs related to the tests. labels May 27, 2023
aduh95
aduh95 reviewed May 27, 2023
View reviewed changes
lib/test/reporters.js Outdated Show resolved Hide resolved
@shubham9411 shubham9411 mentioned this pull request May 27, 2023
Closed
@MoLow MoLow mentioned this pull request May 28, 2023
Closed
aduh95
aduh95 reviewed May 28, 2023
View reviewed changes
lib/test/reporters.js Outdated Show resolved Hide resolved
@MoLow MoLow requested a review from aduh95 June 1, 2023 06:47
@Sumi0
Copy link
Author

Sumi0 commented Jun 1, 2023

I do not understand why is the test failing @line-no:79 test-runner-run.mjs.
Where could be the root-cause ? @MoLow @aduh95

@MoLow
Copy link
Member

MoLow commented Jun 1, 2023

you should debug locally with ./node test/parallel/test-runner-run.mjs

@Sumi0
Copy link
Author

Sumi0 commented Jun 1, 2023

I do not understand why is the test failing @line-no:79 test-runner-run.mjs.
Where could be the root-cause ? @MoLow @aduh95

    not ok 9 - should be piped with spec
      ---
      duration_ms: 1767.493155
      failureType: 'cancelledByParent'
      error: 'Promise resolution is still pending but the event loop has already resolved'
      code: 'ERR_TEST_FAILURE'
      stack: |-
        process.emit (node:events:523:35)
      ...

aduh95
aduh95 reviewed Jun 1, 2023
View reviewed changes
lib/test/reporters.js
Comment on lines +24 to +25
value: spec ?? function SpecReporter() {
return ReflectConstruct(require('internal/test_runner/reporter/spec'), arguments);
Copy link
Contributor

@aduh95 aduh95 Jun 1, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
value: spec ?? function SpecReporter() {
return ReflectConstruct(require('internal/test_runner/reporter/spec'), arguments);
value: function SpecReporter() {
spec ??= require('internal/test_runner/reporter/spec');
return ReflectConstruct(spec, arguments, new.target);

(not sure about the new.target)

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let me run full test this time on my local and check.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@atlowChemi I have tried all versions of returning instance of a class (imported with require), either with new keyword or with Reflect.construct, and both are not working.
I am not able to identify the Root cause.

@atlowChemi atlowChemi mentioned this pull request Jun 30, 2023
Closed
@cjihrig
Copy link
Contributor

cjihrig commented Aug 15, 2023

@Sumi0 are you still planning to work on this, or should it be closed?

@himself65 himself65 mentioned this pull request Oct 13, 2023
Closed
@aduh95 aduh95 added the stalled Issues and PRs that are stalled. label May 11, 2024
@github-actions GitHub Actions
Copy link
Contributor

This issue/PR was marked as stalled, it will be automatically closed in 30 days. If it should remain open, please leave a comment explaining why it should remain open.

@benjamingr
Copy link
Member

This was already superseded by #49184

@benjamingr benjamingr closed this May 11, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@aduh95 aduh95 aduh95 left review comments

@MoLow MoLow MoLow approved these changes

Assignees
No one assigned
Labels
needs-ci PRs that need a full CI run. stalled Issues and PRs that are stalled. test Issues and PRs related to the tests.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Specify that spec reporter is a class and needs to be instantiate for usage with run
6 participants
@Sumi0 @nodejs-github-bot @MoLow @cjihrig @benjamingr @aduh95